Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): allow users to define timing of ViewChild/ContentChild queries #28810

Closed
wants to merge 2 commits into from

Conversation

kara
Copy link
Contributor

@kara kara commented Feb 19, 2019

Prior to this commit, the timing of ViewChild/ContentChild query
resolution depended on the results of each query. If any results
for a particular query were nested inside embedded views (e.g.
*ngIfs), that query would be resolved after change detection ran.
Otherwise, the query would be resolved as soon as nodes were created.

This inconsistency in resolution timing had the potential to cause
confusion because query results would sometimes be available in
ngOnInit, but sometimes wouldn't be available until ngAfterContentInit
or ngAfterViewInit. Code depending on a query result could suddenly
stop working as soon as an *ngIf or an *ngFor was added to the template.

With this commit, users can dictate when they want a particular
ViewChild or ContentChild query to be resolved with the static flag.
For example, one can mark a particular query as static: false to
ensure change detection always runs before its results are set:

@ContentChild('foo', {static: false}) foo !: ElementRef;

This means that even if there isn't a query result wrapped in an
*ngIf or an *ngFor now, adding one to the template later won't change
the timing of the query resolution and potentially break your component.

Similarly, if you know that your query needs to be resolved earlier
(e.g. you need results in an ngOnInit hook), you can mark it as
static: true.

@ViewChild(TemplateRef, {static: true}) foo !: TemplateRef;

Note: this means that your component will not support *ngIf results.

If you do not supply a static option when creating your ViewChild or
ContentChild query, the default query resolution timing will kick in.

Note: This new option only applies to ViewChild and ContentChild queries,
not ViewChildren or ContentChildren queries, as those types already
resolve after CD runs.

@kara kara requested review from a team as code owners February 19, 2019 00:18
@kara kara added state: WIP area: core Issues related to the framework runtime labels Feb 19, 2019
@ngbot ngbot bot added this to the needsTriage milestone Feb 19, 2019
@kara kara added the target: major This PR is targeted for the next major release label Feb 19, 2019
@kara kara force-pushed the static-option branch 2 times, most recently from 5e9ad62 to 2c25c7a Compare February 19, 2019 00:32
@kara kara changed the title feat(core): allow users to define timing of ViewChild/ContentChild queries WIP: feat(core): allow users to define timing of ViewChild/ContentChild queries Feb 19, 2019
…eries

Prior to this commit, the timing of `ViewChild`/`ContentChild` query
resolution depended on the results of each query. If any results
for a particular query were nested inside embedded views (e.g.
*ngIfs), that query would be resolved after change detection ran.
Otherwise, the query would be resolved as soon as nodes were created.

This inconsistency in resolution timing had the potential to cause
confusion because query results would sometimes be available in
ngOnInit, but sometimes wouldn't be available until ngAfterContentInit
or ngAfterViewInit. Code depending on a query result could suddenly
stop working as soon as an *ngIf or an *ngFor was added to the template.

With this commit, users can dictate when they want a particular
`ViewChild` or `ContentChild` query to be resolved with the `static`
flag. For example, one can mark a particular query as `static: false`
to ensure change detection always runs before its results are set:

```ts
@ContentChild('foo', {static: false}) foo !: ElementRef;
```

This means that even if there isn't a query result wrapped in an
*ngIf or an *ngFor now, adding one to the template later won't change
the timing of the query resolution and potentially break your component.

Similarly, if you know that your query needs to be resolved earlier
(e.g. you need results in an ngOnInit hook), you can mark it as
`static: true`.

```ts
@ViewChild(TemplateRef, {static: true}) foo !: TemplateRef;
```

Note: this means that your component will not support *ngIf results.

If you do not supply a `static` option when creating your `ViewChild` or
`ContentChild` query, the default query resolution timing will kick in.

Note: This new option only applies to `ViewChild` and `ContentChild`
queries, not `ViewChildren` or `ContentChildren` queries, as those types
already resolve after CD runs.
@kara kara changed the title WIP: feat(core): allow users to define timing of ViewChild/ContentChild queries feat(core): allow users to define timing of ViewChild/ContentChild queries Feb 19, 2019
@kara kara added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Feb 19, 2019
@jelbourn jelbourn self-requested a review February 19, 2019 18:57
@IgorMinar IgorMinar added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 19, 2019
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm but please document the default.

packages/core/src/metadata/di.ts Outdated Show resolved Hide resolved
packages/core/src/metadata/di.ts Outdated Show resolved Hide resolved
@IgorMinar IgorMinar added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 19, 2019
@kara kara removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 19, 2019
@kara kara removed the request for review from jelbourn February 19, 2019 19:53
@kara kara added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Feb 19, 2019
@kara
Copy link
Contributor Author

kara commented Feb 19, 2019

merge-assistance: global approval from igor/misko

@IgorMinar IgorMinar closed this in 19afb79 Feb 19, 2019
devversion added a commit to devversion/angular that referenced this pull request Feb 26, 2019
Introduces an update schematic for the "@angular/core" package
that automatically migrates pre-V8 "ViewChild" and "ContentChild"
queries to the new explicit timing syntax. This is not required
yet, but with Ivy, queries will be "dynamic" by default. Therefore
specifying an explicit query timing ensures that developers can
smoothly migrate to Ivy (once it's the default).

Read more about the explicit timing API here:
angular#28810
devversion added a commit to devversion/angular that referenced this pull request Feb 26, 2019
Introduces an update schematic for the "@angular/core" package
that automatically migrates pre-V8 "ViewChild" and "ContentChild"
queries to the new explicit timing syntax. This is not required
yet, but with Ivy, queries will be "dynamic" by default. Therefore
specifying an explicit query timing ensures that developers can
smoothly migrate to Ivy (once it's the default).

Read more about the explicit timing API here:
angular#28810
devversion added a commit to devversion/angular that referenced this pull request Feb 26, 2019
Introduces an update schematic for the "@angular/core" package
that automatically migrates pre-V8 "ViewChild" and "ContentChild"
queries to the new explicit timing syntax. This is not required
yet, but with Ivy, queries will be "dynamic" by default. Therefore
specifying an explicit query timing ensures that developers can
smoothly migrate to Ivy (once it's the default).

Read more about the explicit timing API here:
angular#28810
devversion added a commit to devversion/angular that referenced this pull request Feb 26, 2019
Introduces an update schematic for the "@angular/core" package
that automatically migrates pre-V8 "ViewChild" and "ContentChild"
queries to the new explicit timing syntax. This is not required
yet, but with Ivy, queries will be "dynamic" by default. Therefore
specifying an explicit query timing ensures that developers can
smoothly migrate to Ivy (once it's the default).

Read more about the explicit timing API here:
angular#28810
devversion added a commit to devversion/angular that referenced this pull request Feb 26, 2019
Introduces an update schematic for the "@angular/core" package
that automatically migrates pre-V8 "ViewChild" and "ContentChild"
queries to the new explicit timing syntax. This is not required
yet, but with Ivy, queries will be "dynamic" by default. Therefore
specifying an explicit query timing ensures that developers can
smoothly migrate to Ivy (once it's the default).

Read more about the explicit timing API here:
angular#28810
devversion added a commit to devversion/angular that referenced this pull request Feb 27, 2019
Introduces an update schematic for the "@angular/core" package
that automatically migrates pre-V8 "ViewChild" and "ContentChild"
queries to the new explicit timing syntax. This is not required
yet, but with Ivy, queries will be "dynamic" by default. Therefore
specifying an explicit query timing ensures that developers can
smoothly migrate to Ivy (once it's the default).

Read more about the explicit timing API here:
angular#28810
devversion added a commit to devversion/angular that referenced this pull request Mar 1, 2019
Introduces an update schematic for the "@angular/core" package
that automatically migrates pre-V8 "ViewChild" and "ContentChild"
queries to the new explicit timing syntax. This is not required
yet, but with Ivy, queries will be "dynamic" by default. Therefore
specifying an explicit query timing ensures that developers can
smoothly migrate to Ivy (once it's the default).

Read more about the explicit timing API here:
angular#28810
devversion added a commit to devversion/angular that referenced this pull request Mar 5, 2019
Introduces an update schematic for the "@angular/core" package
that automatically migrates pre-V8 "ViewChild" and "ContentChild"
queries to the new explicit timing syntax. This is not required
yet, but with Ivy, queries will be "dynamic" by default. Therefore
specifying an explicit query timing ensures that developers can
smoothly migrate to Ivy (once it's the default).

Read more about the explicit timing API here:
angular#28810
AndrewKushnir pushed a commit that referenced this pull request Mar 5, 2019
)

Introduces an update schematic for the "@angular/core" package
that automatically migrates pre-V8 "ViewChild" and "ContentChild"
queries to the new explicit timing syntax. This is not required
yet, but with Ivy, queries will be "dynamic" by default. Therefore
specifying an explicit query timing ensures that developers can
smoothly migrate to Ivy (once it's the default).

Read more about the explicit timing API here:
#28810

PR Close #28983
@devversion devversion mentioned this pull request Jul 2, 2019
14 tasks
@quangv
Copy link

quangv commented Sep 25, 2019

Why not make this an optional property to avoid breaking changes?

Seems to me that if they had chosen a default behavior, it might have broken some people's expected behavior. Which could be hard to debug.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants